Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify Regex type and rename it to PreventSyncPattern #9049

Merged
merged 6 commits into from
Dec 2, 2024

Conversation

touilleMan
Copy link
Member

No description provided.

@touilleMan touilleMan requested a review from a team as a code owner November 29, 2024 14:20
@touilleMan touilleMan force-pushed the simplify-regex branch 4 times, most recently from d3f86d3 to 424bd3e Compare November 29, 2024 14:55
#[derive(Error, Debug)]
pub enum PreventSyncPatternError {
#[error("Regex parsing error: {err}")]
BadRegex { err: regex::Error },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use tuple style for error since it only has one value.

That simplify errors handling since we can do

.map_error(PreventSyncPattern::BadRegex)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a very good point ! thx

@touilleMan touilleMan force-pushed the simplify-regex branch 2 times, most recently from 16794e0 to ac89683 Compare November 29, 2024 16:31
Copy link
Contributor

@vxgmichel vxgmichel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@touilleMan touilleMan added this pull request to the merge queue Dec 2, 2024
@@ -13,7 +13,7 @@ pub enum ApplyPreventSyncPatternError {
pub(super) async fn ensure_prevent_sync_pattern_applied_to_wksp(
storage: &mut WorkspaceStorage,
device: Arc<LocalDevice>,
pattern: &Regex,
pattern: &PreventSyncPattern,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: rename to prevent_sync_pattern for consistency

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

self.0.iter().any(|r| r.is_match(string))
}

/// Create an empty regex, that regex will never match anything
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Create an empty regex, that regex will never match anything
/// Create an empty PreventSyncPattern, that PreventSyncPattern will never match anything

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 done.

@touilleMan touilleMan removed this pull request from the merge queue due to a manual request Dec 2, 2024
…rs` -> `prevent_sync_pattern.rs` in libparsec_types
…sult}` -> `PreventSyncPattern{Error|Result}`
- Remove useless $ escape code (already handled by `fnmatch_regex`)
- Fix panic in libparsec bindings when config contains invalid pattern
- Remove parsing from file to simplify API
- More consistent naming in methods
- Add more tests \o/
@touilleMan touilleMan enabled auto-merge December 2, 2024 09:39
@touilleMan touilleMan added this pull request to the merge queue Dec 2, 2024
Merged via the queue into master with commit e0fb3e2 Dec 2, 2024
14 checks passed
@touilleMan touilleMan deleted the simplify-regex branch December 2, 2024 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants